Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement vectors without proxy #779

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

deluksic
Copy link
Contributor

@deluksic deluksic commented Jan 14, 2025

Instead of using Proxy to implement swizzling, we can use new Function to compile small getters with no runtime if / switch. This should allow js engines to optimize these calls better, but more testing is needed. The most important thing is that vec[0] and vec.x are simple index access and a getter respectively.

Example swizzle getter:

get xxyw() {
  return new this._Vec4(this[0], this[0], this[1], this[3]);
}

One important change is the ability to unpack vectors like:

const [x, y] = v2

I achieve this by extending Array. Typescript doesn't understand yet that this is possible, but I think we should be able to tell it.

My first tests show almost 2x speedup in buffer.write speed, 25ms vs 40ms for 50k Circle(position, radius, styleIndex) structs.

@deluksic deluksic force-pushed the deluksic/vector-no-proxy branch 2 times, most recently from af23f7e to ca3c797 Compare January 14, 2025 01:20
Copy link

codesandbox-ci bot commented Jan 14, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

}

if (values.length === options.length) {
if (values.length <= 1 || values.length === options.length) {
return options.make(...values);
Copy link
Contributor Author

@deluksic deluksic Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably, we could pass the values directly to the constructor of the vector and not check lengths. In case someone generates thousands of vectors, these checks can add up. We could simply rely on typescript to handle this for us and not pay the runtime cost (even if small).

I think having the option to initialize using vec4(vec3(), w) would be super cool, but I did not find a way to do this without paying the cost of checking args. If someone wants to do this in js, the simplest way would be to vec4(...vec3(), w). This would work just fine, we just need to adjust the transpiler to ignore the dots.

@iwoplaza
Copy link
Collaborator

Awesome initiative! 👏
We have to see if creating functions on-demand works well in Hermes (React Native). If not, we can always do the generation as a build step, and ship the generated code in the package.
I'll take a closer look in the coming days 👀

@deluksic
Copy link
Contributor Author

If it can be autogenerated on build for everyone, that would be even better! Although downsides are code size, and not being able to use the library from node without compile step. (Node has experimental type stripping support)

@iwoplaza
Copy link
Collaborator

To be able to validate if the generation works properly, I'd like to codegen to the file system, and commit. So even if a Node project would like to consume the library from source, it should work okay!

@deluksic
Copy link
Contributor Author

Oooh I see

@deluksic deluksic force-pushed the deluksic/vector-no-proxy branch from ca3c797 to 892aa9c Compare January 15, 2025 22:55
@iwoplaza
Copy link
Collaborator

Okay, I verified that the `new Function(...)` works fine on React Native 🎉. However, doing this at runtime still makes it impossible for things like Static Hermes or Porffor (JS compilers) to optimize them well AOT.

I would still attempt to offload this work AOT as a prepublish step, and ship the generated code. For now though, we can continue experimenting and pushing the performance with a JIT approach here, as it's easier to prototype with.

@iwoplaza
Copy link
Collaborator

iwoplaza commented Feb 3, 2025

Alright, I think if we're able to support both the slow path (proxy) and the fast path (JIT functions), then we're going to have the best of both worlds. We can go back to generating these functions AOT if we find that that's the better approach.

@deluksic Would you be willing to update this PR with a check to see if new Function is supported? And if it's not, use the old Proxy approach? We can check it like so (credit to Brian Breiholz):

let EVAL_ALLOWED_IN_ENV: boolean;
try {
  new Function(`return true`);
  EVAL_ALLOWED_IN_ENV = true;
} catch {
  EVAL_ALLOWED_IN_ENV = false;
}

// ...

@deluksic
Copy link
Contributor Author

deluksic commented Feb 6, 2025

I'm looking at this and it is not super easy to switch this at runtime (not impossible). Are you sure we absolutely must support the proxy way? Do you know when new Function would not be supported where proxy and WebGPU are?

@deluksic
Copy link
Contributor Author

deluksic commented Feb 6, 2025

OK, I found this https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#unsafe_eval_expressions

I will try to find a way to switch at runtime, hopefully by doing this check only once. It kinda defeats the purpose if every vec creation or access needs to check for new Function support.

@deluksic deluksic force-pushed the deluksic/vector-no-proxy branch 2 times, most recently from daa1d2e to 98340f3 Compare February 6, 2025 23:33
@deluksic
Copy link
Contributor Author

deluksic commented Feb 6, 2025

I lied, I implemented the AOT version instead. It was actually easier. For now I created a scripts folder with the script that generates the getters which can be manually copied over to the abstract vector definitions (3 copy paste operations). In the future we can make it automatic, but honestly, this is one of those things you do once.

@deluksic deluksic force-pushed the deluksic/vector-no-proxy branch 3 times, most recently from dbe2cbf to 3927bc2 Compare February 6, 2025 23:40
import type { VecKind } from './wgslTypes';

export abstract class VecBase extends Array implements SelfResolvable {
abstract get kind(): VecKind;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched kind to a getter, so we don't add a string to each instance of the vector.

get wwwx() { return new this._Vec4(this[3], this[3], this[3], this[0]); }
get wwwy() { return new this._Vec4(this[3], this[3], this[3], this[1]); }
get wwwz() { return new this._Vec4(this[3], this[3], this[3], this[2]); }
get wwww() { return new this._Vec4(this[3], this[3], this[3], this[3]); }
Copy link
Contributor Author

@deluksic deluksic Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect this to compress pretty well, but we should check.

@deluksic deluksic force-pushed the deluksic/vector-no-proxy branch from 3927bc2 to 6897d1f Compare February 6, 2025 23:58
@deluksic
Copy link
Contributor Author

deluksic commented Feb 7, 2025

I have an idea for how to reduce the amount of code.

What if we add the 4-way swizzle directly on the VecBase? Sure, some of those getters would not be applicable for Vec2 and Vec3, but also users will never interact with these directly, so typescript would warn anyways. This way, only one manual copy is needed and we remove ~150 LOC.

Added it as an extra commit, we can revert if you don't like this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants